Skip to content

Added a PartialOrder transform #444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 20, 2025
Merged

Added a PartialOrder transform #444

merged 2 commits into from
Apr 20, 2025

Conversation

velochy
Copy link
Contributor

@velochy velochy commented Mar 31, 2025

This is an implementation of a partial order transform as discussed in https://discourse.pymc.io/t/feature-proposal-poset-order-transform/16753/7

I have tried to make it as generic as possible:

  • It takes only the generating DAG and computes the transitive reduction
  • It does not assume the DAG is given in topological order but instead handles sorting internally
  • It converts the matrix to an adjacency list of incoming nodes, reducing computation for low-degree orders
  • It can either take one single DAG for all data or allows specifying a separate DAG for each row

As I am new to contributing to both pymc and pymc-extras, I will likely need some help to get it acceptable.
So far, I have added the code and tests for it, but I don't know how to handle documentation yet.
Also anything else that might need to happen before this can be merged.

@velochy velochy force-pushed the PartialOrder branch 2 times, most recently from 5421a34 to 898c92f Compare April 1, 2025 19:12
@velochy
Copy link
Contributor Author

velochy commented Apr 1, 2025

Added to documentation too now. Please let me know if anything needs doing/improving

@velochy
Copy link
Contributor Author

velochy commented Apr 17, 2025

@zaxtax Yes, let's try to get this one done next :)

I spent some time improving it further, adding a nice example to the documentation.
I can't seem to figure out why the docustring for 'initvals' is not incorporated into the documentation, however. Maybe you can help with that?

@zaxtax
Copy link
Contributor

zaxtax commented Apr 17, 2025 via email

@velochy
Copy link
Contributor Author

velochy commented Apr 17, 2025

I think this relates to a larger refactor of making initvals live more on the sampler than distribution and transform objects pymc-devs/pymc#7528

Interesting development. But I don't think its easy to incorporate transforms into it, as they can interact with the distribution they are transforming in weird ways. Case in point is the one here. For instance, if the underlying distribution has to be positive, we would need initvals that are both positive and respect the partial order provided.

Current solution here is to allow .initvals fn to take lower and upper as parameters, so it can spit out values that are consistent with both. But the user still needs to be able to tell it that this is the case :)

__all__ = ["PartialOrder"]


# Find the minimum value for a given dtype
Copy link
Contributor

@zaxtax zaxtax Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments should probably just be docstrings in the functions

@zaxtax zaxtax self-requested a review April 19, 2025 20:33
Copy link
Contributor

@zaxtax zaxtax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@zaxtax
Copy link
Contributor

zaxtax commented Apr 19, 2025

I can't seem to get this merged. Can you try rebasing from main and hopefully that will unblock this?

@zaxtax zaxtax merged commit be4d843 into pymc-devs:main Apr 20, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants